Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

introduce a freelist interface #775

Merged
merged 1 commit into from
Jul 16, 2024
Merged

introduce a freelist interface #775

merged 1 commit into from
Jul 16, 2024

Conversation

tjungblu
Copy link
Contributor

This introduces an interface for the freelist, splits it into two concrete implementations.

fixes #773

tjungblu
tjungblu commented Jun 25, 2024
View reviewed changes
db_test.go Outdated Show resolved Hide resolved
@tjungblu tjungblu mentioned this pull request Jun 25, 2024
Closed
tjungblu
tjungblu commented Jun 25, 2024
View reviewed changes
internal/freelist/freelist.go
FreePageIds() common.Pgids

// PendingCount returns the number of pending pages.
PendingCount() int
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is unfortunately leaking via stats, ie in db.stats.PendingPageN = db.freelist.PendingCount()

@tjungblu
Copy link
Contributor Author

rebased to the latest freelist refactoring

@tjungblu tjungblu force-pushed the refactor_freelists branch 2 times, most recently from 18a145d to 56634cd Compare June 27, 2024 15:30
@tjungblu tjungblu closed this Jun 28, 2024
@tjungblu tjungblu reopened this Jun 28, 2024
@tjungblu
Copy link
Contributor Author

let's see how the benchmarks perform :)

ahrtr
ahrtr reviewed Jun 28, 2024
View reviewed changes
internal/freelist/hashmap.go Outdated Show resolved Hide resolved
internal/freelist/array.go Outdated Show resolved Hide resolved
internal/freelist/serde.go Outdated Show resolved Hide resolved
internal/freelist/freelist.go Outdated Show resolved Hide resolved
internal/freelist/freelist.go Show resolved Hide resolved
internal/freelist/hashmap.go Outdated Show resolved Hide resolved
internal/freelist/array.go Outdated Show resolved Hide resolved
internal/freelist/hashmap.go Outdated Show resolved Hide resolved
@tjungblu tjungblu force-pushed the refactor_freelists branch 2 times, most recently from 51cbf71 to 7e8d09b Compare June 28, 2024 14:07
ahrtr
ahrtr reviewed Jul 1, 2024
View reviewed changes
internal/freelist/serde.go Outdated Show resolved Hide resolved
@tjungblu tjungblu force-pushed the refactor_freelists branch 2 times, most recently from a78f337 to 7f65806 Compare July 1, 2024 10:27
ahrtr
ahrtr reviewed Jul 1, 2024
View reviewed changes
internal/freelist/freelist.go Outdated Show resolved Hide resolved
@tjungblu tjungblu force-pushed the refactor_freelists branch 2 times, most recently from 30092a9 to 59c498b Compare July 2, 2024 11:49
tjungblu
tjungblu commented Jul 2, 2024
View reviewed changes
internal/freelist/freelist.go Outdated
FreeCount() int

// FreePageIds returns all free pages.
FreePageIds() common.Pgids
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

that can be package private now, used only by ReadWriter and unit tests

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems true. We can just move it right above the pendingPageIds method.

ahrtr
ahrtr reviewed Jul 2, 2024
View reviewed changes
internal/freelist/array.go Outdated Show resolved Hide resolved
internal/freelist/freelist.go Outdated Show resolved Hide resolved
internal/freelist/freelist.go Outdated Show resolved Hide resolved
internal/freelist/freelist.go Outdated Show resolved Hide resolved
internal/freelist/freelist.go Outdated Show resolved Hide resolved
internal/freelist/shared.go Outdated Show resolved Hide resolved
internal/freelist/shared.go Outdated Show resolved Hide resolved
internal/freelist/shared.go Outdated Show resolved Hide resolved
internal/freelist/freelist.go Outdated Show resolved Hide resolved
ahrtr
ahrtr reviewed Jul 2, 2024
View reviewed changes
internal/freelist/array.go Outdated Show resolved Hide resolved
internal/freelist/hashmap.go Outdated Show resolved Hide resolved
ahrtr
ahrtr reviewed Jul 2, 2024
View reviewed changes
internal/freelist/array.go Outdated Show resolved Hide resolved
internal/freelist/hashmap.go Outdated Show resolved Hide resolved
internal/freelist/shared.go Show resolved Hide resolved
internal/freelist/array.go Outdated Show resolved Hide resolved
internal/freelist/hashmap.go Outdated Show resolved Hide resolved
@tjungblu tjungblu force-pushed the refactor_freelists branch 2 times, most recently from 9cca4c6 to d278e30 Compare July 2, 2024 14:10
ahrtr
ahrtr reviewed Jul 2, 2024
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removed freelist_test.go has 485 lines of code, but the sum of the array_test.go, freelist_test.go and hashmap_test.go have around 360 lines of code, where is the remaining around 120 lines of test code?

db.go Outdated Show resolved Hide resolved
@tjungblu
Copy link
Contributor Author

tjungblu commented Jul 2, 2024

The removed freelist_test.go has 485 lines of code, but the sum of the array_test.go, freelist_test.go and hashmap_test.go have around 360 lines of code, where is the remaining around 120 lines of test code?

yep good catch, there were more hashmap tests at the bottom which are now back in action. Line counts are matching again?

@tjungblu tjungblu force-pushed the refactor_freelists branch 2 times, most recently from d9c0ee0 to b3019ed Compare July 2, 2024 15:29
@ahrtr
Copy link
Member

ahrtr commented Jul 2, 2024

yep good catch, there were more hashmap tests at the bottom which are now back in action. Line counts are matching again?

Yes, makes sense now. thx.

One minor comment on the order of the methods in shared.go, please try to reorder the methods, e.g.

  • put release and releaseRange right below ReleasePendingPages;
  • put Read, EstimatedWritePageSize and Write at the bottom of the file;
  • put txIDx and its implementation on sort.Interface right above ReleasePendingPages

@tjungblu tjungblu force-pushed the refactor_freelists branch 2 times, most recently from 3313cf5 to c5576f1 Compare July 2, 2024 15:48
@tjungblu
Copy link
Contributor Author

tjungblu commented Jul 2, 2024

done, PTAL again

ahrtr
ahrtr approved these changes Jul 2, 2024
View reviewed changes
Copy link
Member

@ahrtr ahrtr left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great work! Thanks @tjungblu .

We can continue to think about how to add dedicated & randomized test case for the freelist management. We can also continue to refactor if needed.

cc @fuweid to take a look

@tjungblu
Copy link
Contributor Author

tjungblu commented Jul 2, 2024

Thanks for your patience! I think the best step forward is to test the interface for starters. Then we can check what else we can refactor from there...

@tjungblu
Copy link
Contributor Author

tjungblu commented Jul 8, 2024

@fuweid any comments?

This was referenced Jul 8, 2024
Merged
Open
@tjungblu
introduce a freelist interface
62e81c0 
This introduces an interface for the freelist, splits it into two concrete
implementations.

fixes etcd-io#773

Signed-off-by: Thomas Jungblut <[email protected]>
@fuweid
Copy link
Member

fuweid commented Jul 9, 2024

@tjungblu I will take a look in following two days. sorry for the delay.

@tjungblu tjungblu mentioned this pull request Jul 9, 2024
Merged
fuweid
fuweid approved these changes Jul 14, 2024
View reviewed changes
Copy link
Member

@fuweid fuweid left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

/lgtm
/approve

internal/freelist/shared.go

// Copy the list of page ids from the freelist.
if len(ids) == 0 {
t.Init(nil)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For array type, the original implementation just resets ids. In this patch, array type will updates t.cache as well. It might involve some memory allocation but it looks fine.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yep, I found some inconsistencies also during unit testing between the default implementations init (empty vs. nil). We'll fix them along with the tests.

internal/freelist/freelist.go
NoSyncReload(pgIds common.Pgids)

// freePageIds returns the IDs of all free pages.
freePageIds() common.Pgids
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

These non-accessable methods could be considered to remove from Interface and we can add a new non-accessable interface for array type and hash type, like.

type pageIDManager interface {
        freePageIds() common.Pgids
        pendingPageIds() map[common.Txid]*txPending
        release(txId common.Txid)
        releaseRange(begin, end common.Txid)
        mergeSpans(ids common.Pgids)
}

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can continue to refactor the interface later.

@k8s-ci-robot
Copy link

[APPROVALNOTIFIER] This PR is APPROVED

This pull-request has been approved by: ahrtr, fuweid, tjungblu

The full list of commands accepted by this bot can be found here.

The pull request process is described here

Needs approval from an approver in each of these files:

Approvers can indicate their approval by writing /approve in a comment
Approvers can cancel approval by writing /approve cancel in a comment

@ahrtr ahrtr merged commit efc3eb6 into etcd-io:main Jul 16, 2024
19 checks passed
@ahrtr
Copy link
Member

ahrtr commented Jul 16, 2024

Link to #789

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@ahrtr ahrtr ahrtr approved these changes

@fuweid fuweid fuweid approved these changes

Assignees

@fuweid fuweid

Labels
approved lgtm size/XXL
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Introduce freelist interface
4 participants
@tjungblu @ahrtr @fuweid @k8s-ci-robot